Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow volume attach limit overwrite via command line parameter #522

Merged
merged 1 commit into from
Jun 29, 2020
Merged

Allow volume attach limit overwrite via command line parameter #522

merged 1 commit into from
Jun 29, 2020

Conversation

rfranzke
Copy link
Contributor

@rfranzke rfranzke commented Jun 8, 2020

Is this a bug fix or adding new feature?
/kind feature

What is this PR about? / Why do we need it?
This PR is a part of #347 and proceeds with the way suggested in #347 (comment): In order to allow existing clusters that are leveraging/relying on the KUBE_MAX_PD_VOLS feature for in-tree provisioners in Kubernetes (https://kubernetes.io/docs/concepts/storage/storage-limits/#custom-limits) to migrate to CSI, the EBS CSI driver is supporting the --volume-attach-limit-overwrite flag now until a more sophisticated solution can be implemented.

PS: I was adding a few missing doc strings along the way and reduced package/function name stuttering according to the official Go suggestions. Hope that's fine.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 8, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @rfranzke. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 8, 2020
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 8, 2020
@rfranzke
Copy link
Contributor Author

rfranzke commented Jun 8, 2020

/assign @leakingtapan

@rfranzke
Copy link
Contributor Author

rfranzke commented Jun 8, 2020

cc @gnufied

@coveralls
Copy link

coveralls commented Jun 8, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 1125

  • 7 of 14 (50.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.7%) to 79.577%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/driver/driver.go 4 6 66.67%
pkg/driver/node.go 3 8 37.5%
Files with Coverage Reduction New Missed Lines %
pkg/driver/node.go 1 69.44%
Totals Coverage Status
Change from base Build 1111: 0.7%
Covered Lines: 1430
Relevant Lines: 1797
💛 - Coveralls

@rfranzke rfranzke force-pushed the feature/volume-limit-overwrite branch from 59f1ab9 to 426c3e5 Compare June 8, 2020 13:40
@gnufied
Copy link
Contributor

gnufied commented Jun 11, 2020

/ok-to-test

@k8s-ci-robot
Copy link
Contributor

@gnufied: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hardikdr
hardikdr approved these changes Jun 11, 2020
View reviewed changes
Copy link
Member

@hardikdr hardikdr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, just a minor comment.

cmd/options/node_options.go Outdated Show resolved Hide resolved
@hardikdr
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 11, 2020
@hardikdr
Copy link
Member

@leakingtapan @wongma7 It would be helpful if you could also take a quick look.

It seems like a reasonable change, to not break the user experience with CSI migration on AWS/EKS. Basically the KUBE_MAX_PD_VOLS on kube-scheduler is ineffective with CSI-drivers and leaves no way for end-user to overwrite max-attachable-volumes.

  • This PR at least helps in keeping existing experience intact.

@wongma7
Copy link
Contributor

wongma7 commented Jun 11, 2020

Giving @leakingtapan a chance to chime in as he had some objections to this but I agree it's necessary to make the limit configurable somehow and a flag seems the best way.

@rfranzke
Copy link
Contributor Author

@leakingtapan Do you have some time to check the PR? Any thoughts on it?

@rfranzke
Copy link
Contributor Author

rfranzke commented Jun 22, 2020 •
edited
Loading

Hm, what's the general review process now? As @leakingtapan seems to be busy/inactive, can another owner/approver help to progress with this PR? @bertinatto @jsafrane @gnufied @wongma7

gnufied
gnufied reviewed Jun 22, 2020
View reviewed changes
Copy link
Contributor

@gnufied gnufied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good to me.

cmd/options/controller_options.go Outdated Show resolved Hide resolved
cmd/options/node_options.go Outdated Show resolved Hide resolved
cmd/options/server_options.go Outdated Show resolved Hide resolved
pkg/driver/driver.go Outdated Show resolved Hide resolved
@rfranzke rfranzke force-pushed the feature/volume-limit-overwrite branch from 426c3e5 to 0b554b6 Compare June 23, 2020 05:30
@rfranzke
Copy link
Contributor Author

/retest

@rfranzke
Copy link
Contributor Author

@gnufied I've dropped all unrelated changes.

gnufied
gnufied reviewed Jun 23, 2020
View reviewed changes
docs/design.md Outdated Show resolved Hide resolved
@rfranzke
Copy link
Contributor Author

Btw, any pointers on how to get the e2e tests fixed? They fail unrelatedly, see https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-ebs-csi-driver/522/pull-aws-ebs-csi-driver-e2e-multi-az/1275303611722633217/build-log.txt.

$ curl https://sum.golang.org/lookup/github.com/aws/[email protected] -I
HTTP/2 410

@rfranzke rfranzke requested a review from hardikdr June 24, 2020 08:27
@gnufied
Copy link
Contributor

gnufied commented Jun 24, 2020

Looks good to me but we might want to wait for @leakingtapan feedback too.

/lgtm
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2020
@k8s-ci-robot
Copy link
Contributor

@gnufied: changing LGTM is restricted to collaborators

In response to this:

Looks good to me but we might want to wait for @leakingtapan feedback too.

/lgtm
/hold

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hardikdr
hardikdr approved these changes Jun 24, 2020
View reviewed changes
Copy link
Member

@hardikdr hardikdr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2020
@gnufied
Copy link
Contributor

gnufied commented Jun 24, 2020

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2020
leakingtapan
leakingtapan reviewed Jun 26, 2020
View reviewed changes
Copy link
Contributor

@leakingtapan leakingtapan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the effort to improve current situation. Agree that's the best option we have now. Left one comment

pkg/driver/node.go Outdated Show resolved Hide resolved
@rfranzke rfranzke force-pushed the feature/volume-limit-overwrite branch from b9b1662 to 56e2559 Compare June 29, 2020 06:03
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2020
@rfranzke rfranzke requested a review from leakingtapan June 29, 2020 06:03
@rfranzke rfranzke force-pushed the feature/volume-limit-overwrite branch from 56e2559 to 2d9fe32 Compare June 29, 2020 06:24
@rfranzke
Copy link
Contributor Author

/retest

2 similar comments
@rfranzke
Copy link
Contributor Author

/retest

@rfranzke
Copy link
Contributor Author

/retest

@gnufied
Copy link
Contributor

gnufied commented Jun 29, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 29, 2020
@leakingtapan
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, leakingtapan, rfranzke

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [gnufied,leakingtapan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 563a72b into kubernetes-sigs:master Jun 29, 2020
@ialidzhikov ialidzhikov mentioned this pull request Aug 12, 2020
Merged
1 task
@rfranzke rfranzke deleted the feature/volume-limit-overwrite branch September 1, 2020 14:43
@gnufied gnufied mentioned this pull request Oct 1, 2020
Closed
@sjmiller609 sjmiller609 mentioned this pull request Dec 1, 2021
Merged
@sjmiller609
Copy link

sjmiller609 commented Dec 1, 2021 •
edited
Loading

this is configurable in the helm chart here

@ffilippopoulos ffilippopoulos mentioned this pull request Mar 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnufied gnufied gnufied left review comments

@wongma7 wongma7 wongma7 approved these changes

@hardikdr hardikdr hardikdr approved these changes

@leakingtapan leakingtapan Awaiting requested review from leakingtapan

Assignees

@leakingtapan leakingtapan

@hardikdr hardikdr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rfranzke @k8s-ci-robot @coveralls @gnufied @hardikdr @wongma7 @leakingtapan @sjmiller609